Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use encrypted auth token for images #3597

Merged
merged 5 commits into from
Jun 22, 2021
Merged

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jun 15, 2021

Details

Uses an encrypted auth token instead of a plain one in URLs to fetch images.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/166503

Tests / QA Steps

  1. Upload an image in a chat.
  2. Verify that you can view the thumbnail image in the chat.
  3. Tap on the image, and verify that you can view the full-size preview in the attachment modal.
  4. Make sure you are not logged into E.com.
  5. Upload a .zip file to chat.
  6. Click on the attachment link for the .zip file, verify that it is downloaded without logging into E.com.
  7. Log out then back in, then repeat steps 2, 3, & 6
  8. Open the developer console, go to Application, search for the session key, and erase the key-value pair for encryptedAuthToken. Then refresh the page and repeat steps 2, 3, & 6.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image
image

Mobile Web

Desktop

iOS

Android

@roryabraham roryabraham requested a review from cead22 June 15, 2021 20:36
@roryabraham roryabraham requested a review from a team as a code owner June 15, 2021 20:36
@roryabraham roryabraham self-assigned this Jun 15, 2021
@MelvinBot MelvinBot requested review from mountiny and removed request for a team June 15, 2021 20:36
@roryabraham
Copy link
Contributor Author

@roryabraham roryabraham changed the title Use encrypted auth token for images [HOLD Web-E #31211] Use encrypted auth token for images Jun 15, 2021
@roryabraham
Copy link
Contributor Author

Added an Onyx migration so users don't have to sign out and back in for images to work.

@roryabraham
Copy link
Contributor Author

Oops, I introduced a bug with the migration.

@mountiny
Copy link
Contributor

In case it is different problem than you have. The branch should probably not work yet as mentioned above, but for me it keeps hanging on blank page after uploading an image. The thumbnail was loading for a long time so I refreshed the page and I get a blank blue page and the only way to make it work is to Clear site data in the inspector.

src/libs/actions/Session.js Show resolved Hide resolved
src/libs/addEncryptedAuthTokenToURL.js Outdated Show resolved Hide resolved
src/libs/migrations/AddEncryptedAuthToken.js Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor Author

for me it keeps hanging on blank page after uploading an image

Good call out @mountiny, thanks for testing! This helped me identify a bug in the migration, which is now fixed. For posterity, the two issues here were:

  1. (Web-E) The encryptedAuthToken needed to be url-encoded before being sent across the network.
  2. I was missing a resolve() in the migration so the promise was hanging.

@roryabraham roryabraham requested a review from cead22 June 17, 2021 23:43
cead22
cead22 previously approved these changes Jun 18, 2021
src/libs/actions/Session.js Outdated Show resolved Hide resolved
src/libs/addEncryptedAuthTokenToURL.js Outdated Show resolved Hide resolved
src/libs/migrations/AddEncryptedAuthToken.js Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor Author

Updated!

@mountiny
Copy link
Contributor

As the HOLD PR https://github.com/Expensify/Web-Expensify/pull/31211 is merged, shall I merge it or do I need to wait for the Title label to be updated?

@roryabraham roryabraham changed the title [HOLD Web-E #31211] Use encrypted auth token for images Use encrypted auth token for images Jun 18, 2021
@roryabraham roryabraham changed the title Use encrypted auth token for images [HOLD] Use encrypted auth token for images Jun 18, 2021
@roryabraham
Copy link
Contributor Author

@mountiny Good question – general rule-of-thumb is don't merge a PR with HOLD in the title. In this case we need to wait for that Web-E PR to be deployed to production.

@mountiny
Copy link
Contributor

Perfect! Thanks for clarifying!

@roryabraham roryabraham changed the title [HOLD] Use encrypted auth token for images Use encrypted auth token for images Jun 22, 2021
@roryabraham roryabraham merged commit 76c3157 into main Jun 22, 2021
@roryabraham roryabraham deleted the Rory-UseEncryptedAuthToken branch June 22, 2021 23:24
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.73-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants